Review #69
Review #69
Conversation
|
Apart from the comments and suggestions in the Files changed section, I'm also curious about something. This module essentially enables governance to take tokens from any address. What about situations where this is misused such as to move tokens out of a whale's address into somewhere else? Of course this will require governance to agree to do this, but the large amount of tokens in the whale's address might be a big enough incentive. |
|
Any fixes that are agreed-upon should be moved to a separate PR given that this PR includes both comments and changes |
| @@ -25,4 +28,7 @@ type AccountKeeper interface { | |||
| GetModuleAddress(name string) sdk.AccAddress | |||
| GetModuleAccount(ctx sdk.Context, moduleName string) authtypes.ModuleAccountI | |||
| SetModuleAccount(sdk.Context, authtypes.ModuleAccountI) | |||
|
|
|||
| // Note that GetAccount function is not used anywhere in the project. | |||
| // Consider limiting AccountKeeper expected functions to only these used functions. | |||
There was a problem hiding this comment.
Thanks for the suggestion It will be applied in #76.
There was a problem hiding this comment.
Appreciate for the review @migueldingli1997. The documentation related feedbacks will be applied in #71.
@migueldingli1997 You're understanding how the module works correctly. It requires governance to agree to do it. We design the module excluding the possibility for the community to agree such proposal and it gets passed. Any thoughts from @dlguddus @dongsam will be helpful here. |
| // ^ We can avoid having to validate all budgets at every BeginBlock since | ||
| // we already know that the budgets are valid from the validation of params |
There was a problem hiding this comment.
Thanks for the suggestion, It will be applied on #83.
| if err == nil && !budget.Expired(ctx.BlockTime()) { | ||
| // ^ We can consider automatically deleting expired budgets, instead of | ||
| // expecting this to be done through governance proposals. |
There was a problem hiding this comment.
Thanks for the suggestion, I think the number of budgets will be relatively small, and even if it is over, the past details may be important, so I think it would be better to leave as ended proposals remain in the gov. and it may be burdensome to change params data without gov IMO.
Of course, as you said, it can act as a malicious attack vector, but governance takes two weeks, and in the meantime, if the whale finds the possibility of its address being used as a budget source, it's enough time to move the asset to another wallet, Another usecase can also be used as a means to move the asset from a wallet that has lost its private key. However, it is true that this can confuse other module ecosystems. However, in order to use inflation as a budget source, It is the biggest purpose and a primary use case of the module. this was the best to develop it without maximum dependence on the current sdk module structure, where assets minted in the FeeCollector are collected. If the mint and distribution module are improved in a more general direction in the future, the budget module will also be modified accordingly. |
|
Close the issue because all suggestions have been discussed or resolved. Thank you again. |
Description
Code review of the entire module. Refer to the Files changed for most of the review comments.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.Code follows the module structure standards.Wrote unit and integrationUpdated relevant documentation (docs/) or specification (x/<module>/spec/)Added relevantgodoccomments.Files changedin the Github PR explorerReviewCodecov Reportin the comment section below once CI passes